Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run linters during CI #286

Open
wants to merge 17 commits into
base: canon
Choose a base branch
from
Open

Run linters during CI #286

wants to merge 17 commits into from

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented May 28, 2019

This adds a lint task in CI, which uses the same linters & configuration as ppb-vector.
It is a follow-up on #285, which fixes the warts found by those linters.

  • flake8;
  • flake8-bugbear, which looks for likely bugs and design problems;
  • flake8-commas, for enforcing trailing commas in multi-line dicts, lists, tuples, ...
  • flake8-import-order, for enforcing the import conventions we already use;
  • pep8-naming, for enforcing PEP8's naming conventions.

In the few places where it was necessary, noqa annotations were used, scoped to a specific lint.

@nbraud nbraud requested a review from a team as a code owner May 28, 2019 04:02
@nbraud nbraud changed the title Run linters turing CI Run linters during CI May 28, 2019
@pathunstrom pathunstrom changed the base branch from master to canon June 27, 2020 11:02
@gideongrinberg
Copy link
Contributor

@nbraud does it make sense to lint builds or to use pre-commit hooks? These will prevent poor code from being committed. You can also set up pre-commit to run a formatter like black or autopep8 (although I know there are mixed opinions about formatters.) I'm not sure exactly what the differences are, but it might be worth looking at. It probably comes down to performance and convenience.

@AstraLuma
Copy link
Member

Only if people have installed the additional tools. We have a strong desire to be as friendly as possible for people to get started contributing.

The problems with this aren't "how to run the tool" but "strategy for bringing the code base into compliance".

@gideongrinberg
Copy link
Contributor

Only if people have installed the additional tools. We have a strong desire to be as friendly as possible for people to get started contributing.

That makes a lot of sense Astra. The thing is (and this is just my opinion) linting before you push is important. It enforces good habits, makes the dev focus on readability, and makes code review easier. While it could increase the barrier to entry, most people are just running pip install -r requirements.txt anyway and pre-commit works automatically. It could make it harder for beginners to write code but, as I said, good quality should be enforced. What would be your opinion on formatting in the CI builds?

@AstraLuma
Copy link
Member

The ci check stays.

If the tool is that automatic, can you open an issue for us to discuss it? I want to make sure I understand how well it works in various scenarios.

@gideongrinberg
Copy link
Contributor

@AstraLuma see #531

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants